vello_cpu: Unify render APIs#1665
Conversation
|
@nicoburns If you want to feel free to give it a spin. |
2846da5 to
5b7d009
Compare
nicoburns
left a comment
There was a problem hiding this comment.
I'm onboard with the general concept, but I think it could use some tweaks.
Eventually I would keen on:
- Adding PixmapRef
- Moving all of the Pixmap types into their own crate (so that things like
glifocould use them without depending on `vello_common)
| /// Buffer of the pixmap in RGBA8 format. | ||
| buf: &'a mut [u8], |
There was a problem hiding this comment.
Probably not blocking, but it's a little weird that Pixmap uses PremulRgba8 whereas PixmapMut uses u8 (my preference would be u8 everywhere).
| if settings.composite_mode == CompositeMode::Replace && !target_fully_covered { | ||
| target.data_mut().fill(0); | ||
| } |
There was a problem hiding this comment.
I wouldn’t expect the target would be cleared beforehand when using CompositeMode::Replace. I’d interpret that mode as “replace the rendered region with the scene”, not “wipe everything and then draw the scene”. Also, don’t we already clear regions later with fine.clear? Is Replace equivalent to the Copy blend mode? If so, could we just use the two Porter-Duff operators here and leave it up to the consumer to decide whether the pixmap should be cleared beforehand?
There was a problem hiding this comment.
We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).
There was a problem hiding this comment.
Also, don’t we already clear regions later with fine.clear?
We do, but if the scene doesn't cover the whole pixmap, as I mentioned this will leave the outside regions in the existing pixmap untouched. This is why this action is only triggered if !target_fully_covered.
There was a problem hiding this comment.
We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).
Personally, I would expect Replace to behave as a region-local operator: the rendered region overwrites the destination, while pixels outside that region are left untouched. If the caller really wants a whole-buffer wipe, they can do that explicitly on their side with pixmap.data_as_u8_slice_mut().fill(0). That makes the behaviour opt-in, explicit, and preserves the same cost.
There may also be cases where you want to avoid the cost of SrcOver, especially with unpacking wide tile buffers, but still want to replace only the rendered region without clearing the whole buffer, for example when rendering into a preprocessed image, frame, or another existing target.
b954db3 to
a1237b9
Compare
|
I would also ideally like to test this out, but I'm onboard with the API in principle. |
|
Anything needed from my side to facilitate this, or do you mean “I’ll test this out once I find the time to do so”? |
|
I mean “I’ll test this out once I find the time to do so”. I would also be happy for this to land before that if you're confident in it. |
a1237b9 to
bf1b9df
Compare
|
I agree that the pixmap API should probably be refactored a bit (I'm leaning towards drawing inspiration from tiny-skia here), but I don't want to make this PR harder to review than it already is, so I will leave this for a follow-up. |
e6087b3 to
90e5cb5
Compare
90e5cb5 to
bd98e50
Compare
nicoburns
left a comment
There was a problem hiding this comment.
I definitely have some quibbles with the API, but I've tested this against Blitz and it works fine (no unexpected surprises). So happy for this to land :)
I'd quite like to see |
|
Yeah, as I mentioned, happy to iterate further in a future PR! (Will still wait for Alex to have had a chance to take a look before merging.) |
grebmeg
left a comment
There was a problem hiding this comment.
LGTM
TODO: I still want to run this version against my PDF test suite, but this shouldn't block a review.
Before merging, could you please verify this if you haven’t already?
| if settings.composite_mode == CompositeMode::Replace && !target_fully_covered { | ||
| target.data_mut().fill(0); | ||
| } |
There was a problem hiding this comment.
We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).
Personally, I would expect Replace to behave as a region-local operator: the rendered region overwrites the destination, while pixels outside that region are left untouched. If the caller really wants a whole-buffer wipe, they can do that explicitly on their side with pixmap.data_as_u8_slice_mut().fill(0). That makes the behaviour opt-in, explicit, and preserves the same cost.
There may also be cases where you want to avoid the cost of SrcOver, especially with unpacking wide tile buffers, but still want to replace only the rendered region without clearing the whole buffer, for example when rendering into a preprocessed image, frame, or another existing target.
Already did yesterday, works fine! |
Hmm, okay. Let's do it this way: I'll add a TODO for this now to avoid additional churn for #1701, and once that has landed I will try to make that change on top of that. I think it shouldn't be too hard, but I want to avoid making rebasing harder than it needs to be. 😄 I hope that's okay with you. |
Supersedes #1597, closes #1382.
Motivation
Right now, we have two rendering APIs:
render_to_pixmapandcomposite_to_pixmap_at_offset. We are about to add a third one in #1597. Having this many APIs is very confusing, and after thinking about this more closely I realized that really, what we have been doing is adding methods that do similar things but just with different knobs enabled/disabled. I think it would be much cleaner if we had one single API entry point for rendering to a pixmap, and instead expose the available knobs as a simple settings struct.Implementation
Therefore, this PR proposes to:
rendermethod that can be used to render the contents of aRenderContextintoPixmap-like struct.PixmapMutstruct to allow constructing a render target from a mutable borrowed byte slice. Letrenderalways take aPixmapMut, allowing to pass both, a custom buffer or an owned pixmap as the render target.CompositeModeenum that defines whether the contents of a pixmap should be completely replaced (by default), or whether source-over compositing should be used.PixelFormatstruct, to possibly allowBgra8in the future, which is useful for MacOS.RenderModehas been moved from being stored inRenderContextto instead being a setting during rasterization. I initially stored this inRenderContextbecause I thought it might happen that the setting will also be needed during scene construction, but this turned out to not be the case in the end.RenderContextandPixmapalways need to have the same size, although it "accidentally" was possible for them to not have the same size, and some people have made use of that. This PR relaxes the conditions imposed on the size, explicitly allowing size mismatches and also documents what the intended semantics are for each.Testing
I've added some new tests for the expected behavior. Existing tests pass. I've also ran
vello_cpu_winitwith the different scenes and didn't notice any regressions.TODO: I still want to run this version against my PDF test suite, but this shouldn't block a review.